Improve diagnostics for parenthesized type arguments #122152

wutchzone · 2024-03-07T19:09:31Z

Fixes #120892

r? fmease

rustbot · 2024-03-07T19:09:40Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @fmease (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

fmease

Thank you for working on this! Looks very good already. I have a couple of suggestions. I will probably kick off a performance run once you have address my suggestions related to the parser snapshot creation to make sure we don't impact the parsing performance of Fn{,Mut,Once}(…) -> … in the good path.

fmease · 2024-03-08T12:39:20Z

compiler/rustc_parse/src/parser/path.rs

+
+                    let mut snapshot = None;
+                    if self.may_recover()
+                        && (self.token.can_begin_expr() || self.token.can_begin_pattern())


Could you make this check stricter by only checking if style is PathStyle::Expr first and if so if token.can_begin_expr(), too, or if style is PathStyle::Pat and if so if token.can_begin_pattern()?

compiler/rustc_parse/src/parser/path.rs

fmease · 2024-03-08T12:57:54Z

compiler/rustc_parse/src/parser/path.rs

+                    let (inputs, _) = match self.parse_paren_comma_seq(|p| p.parse_ty()) {
+                        Ok(output) => output,
+                        Err(mut error) if prev_token_before_parsing.kind == token::ModSep => {
+                            error.span_label(prev_token_before_parsing.span.with_hi(prev_token_before_parsing.span.hi() + BytePos(1)), "while parsing this list of parenthesized type arguments starting here");


Manual offsetting is a bit fragile. I don't think this specific instance can lead to crashes but SourceMap::next_point should be preferred over BytePos(1) to account for multi-byte Unicode code points.

Anyway, BytePos(1) is not what we want here because it doesn't work if we have f:: ( (notice the space between the path separator and the opening parenthesis. Could you instead use Span::until or Span::between where the “upper”/right span is the span of the opening parenthesis / the span of the “paren_comma_seq” (I don't know how easy it is to obtain) to highlight the entirety of e.g. :: ( or ::/* comment */(.

You are right, I absolutely missed that 🤦 . I added to the tests. I think it should be enough to save the current token before calling the parse_paren_comma_seq with a combination of Span::to.

Now I wonder how fragile is the hardcoded byte offsetting, there are plenty of them in the code base 😄 .

compiler/rustc_parse/src/parser/path.rs

fmease · 2024-03-08T13:02:53Z

compiler/rustc_parse/src/parser/path.rs

+                                    {
+                                        error.span_suggestion_verbose(
+                                            prev_token_before_parsing.span,
+                                            "consider removing `::`",


The suggestion is already pretty good but I'd like to see more context provided in this message. Something like consider removing the `::` here to make this a function call / … to turn this into a …. Function call is definitely not the right term here, maybe call expression / tuple struct pattern depending on the style.

Hopefully you can find a better more general term here, I don't know how to best call “$expr($expr, …)” / $path($pat, …) lol.

I think™ the call expression is the correct term based on the rust reference. For the second one, I couldn't find a better name than you had already suggested.

compiler/rustc_parse/src/parser/path.rs

tests/ui/parser/issues/diagnostics-parenthesized-type-arguments-issue-120892-3.stderr

wutchzone · 2024-03-09T13:42:31Z

@rustbot label -S-waiting-on-author +S-waiting-on-review

fmease · 2024-03-09T16:08:17Z

tests/ui/parser/issues/diagnostics-parenthesized-type-arguments-issue-120892-2.stderr

+error: expected type, found `123`
+  --> $DIR/diagnostics-parenthesized-type-arguments-issue-120892-2.rs:2:45
+   |
+LL |   foo::/* definitely not harmful comment */(123, "foo") -> (u32);


fmease · 2024-03-09T16:08:51Z

compiler/rustc_parse/src/parser/path.rs

+                format!(
+                    "consider removing the `::` here to {}",
+                    match style {
+                        PathStyle::Expr => "call the expresion",


Suggested change

PathStyle::Expr => "call the expresion",

PathStyle::Expr => "call the expression",

typo

fmease · 2024-03-09T16:10:59Z

Let's do a perf run to make sure we haven't regressed the parsing perf of FnOnce() and friends.
@bors try @rust-timer queue

Improve diagnostics for parenthesized type arguments Fixes rust-lang#120892 r? fmease

bors · 2024-03-09T16:12:10Z

⌛ Trying commit 2648a13 with merge d180540...

bors · 2024-03-09T17:41:01Z

☀️ Try build successful - checks-actions
Build commit: d180540 (d180540fce069d2383eb1d1812142eff89641701)

rust-timer · 2024-03-09T19:41:06Z

Finished benchmarking commit (d180540): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.4%	[-0.5%, -0.3%]	3
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.8%	[0.8%, 0.8%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.8%	[0.8%, 0.8%]	1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 649.076s -> 648.931s (-0.02%)
Artifact size: 172.58 MiB -> 308.34 MiB (78.66%)

Kobzol · 2024-03-10T10:26:48Z

(The artifact size change is just getting the libLLVM.so size back, thanks to rust-lang/rustc-perf#1838).

fmease

I love it, thanks!

fmease · 2024-03-11T11:45:07Z

@bors r+ rollup

bors · 2024-03-11T11:45:10Z

📌 Commit 58f6aaa has been approved by fmease

It is now in the queue for this repository.

Improve diagnostics for parenthesized type arguments Fixes rust-lang#120892 r? fmease

…kingjubilee Rollup of 15 pull requests Successful merges: - rust-lang#116791 (Allow codegen backends to opt-out of parallel codegen) - rust-lang#116793 (Allow targets to override default codegen backend) - rust-lang#117458 (LLVM Bitcode Linker: A self contained linker for nvptx and other targets) - rust-lang#119385 (Fix type resolution of associated const equality bounds (take 2)) - rust-lang#121438 (std support for wasm32 panic=unwind) - rust-lang#121893 (Add tests (and a bit of cleanup) for interior mut handling in promotion and const-checking) - rust-lang#122080 (Clarity improvements to `DropTree`) - rust-lang#122152 (Improve diagnostics for parenthesized type arguments) - rust-lang#122166 (Remove the unused `field_remapping` field from `TypeLowering`) - rust-lang#122249 (interpret: do not call machine read hooks during validation) - rust-lang#122299 (Store backtrace for `must_produce_diag`) - rust-lang#122318 (Revision-related tweaks for next-solver tests) - rust-lang#122320 (Use ptradd for vtable indexing) - rust-lang#122328 (unix_sigpipe: Replace `inherit` with `sig_dfl` in syntax tests) - rust-lang#122330 (bootstrap readme: fix, improve, update) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#122152 - wutchzone:120892, r=fmease Improve diagnostics for parenthesized type arguments Fixes rust-lang#120892 r? fmease

rustbot assigned fmease Mar 7, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 7, 2024

This comment has been minimized.

Sign in to view

wutchzone force-pushed the 120892 branch from 1d23ada to 99ca30a Compare March 7, 2024 19:35

fmease reviewed Mar 8, 2024

View reviewed changes

fmease added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 8, 2024

fmease reviewed Mar 8, 2024

View reviewed changes

tests/ui/parser/issues/diagnostics-parenthesized-type-arguments-issue-120892-3.stderr Outdated Show resolved Hide resolved

wutchzone force-pushed the 120892 branch from 99ca30a to 2648a13 Compare March 9, 2024 12:12

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 9, 2024

fmease reviewed Mar 9, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 9, 2024

Auto merge of rust-lang#122152 - wutchzone:120892, r=<try>

d180540

Improve diagnostics for parenthesized type arguments Fixes rust-lang#120892 r? fmease

This comment has been minimized.

Sign in to view

Improve diagnostics for parenthesized type arguments

58f6aaa

wutchzone force-pushed the 120892 branch from 2648a13 to 58f6aaa Compare March 9, 2024 21:16

fmease approved these changes Mar 11, 2024

View reviewed changes

bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 11, 2024

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Mar 11, 2024

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 11, 2024

Rollup merge of rust-lang#122152 - wutchzone:120892, r=fmease

23e4968

Improve diagnostics for parenthesized type arguments Fixes rust-lang#120892 r? fmease

matthiaskrgr mentioned this pull request Mar 11, 2024

Rollup of 11 pull requests #122337

Closed

workingjubilee mentioned this pull request Mar 11, 2024

Rollup of 15 pull requests #122338

Merged

bors merged commit 05ff86c into rust-lang:master Mar 11, 2024
11 checks passed

rustbot added this to the 1.78.0 milestone Mar 11, 2024

rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 11, 2024

Unrolled build for rust-lang#122152

c656bd6

Rollup merge of rust-lang#122152 - wutchzone:120892, r=fmease Improve diagnostics for parenthesized type arguments Fixes rust-lang#120892 r? fmease

matthiaskrgr mentioned this pull request Mar 11, 2024

ice: error was constructed but not emitted #122345

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve diagnostics for parenthesized type arguments #122152

Improve diagnostics for parenthesized type arguments #122152

wutchzone commented Mar 7, 2024

rustbot commented Mar 7, 2024

This comment has been minimized.

fmease left a comment •

edited

Loading

fmease Mar 8, 2024

wutchzone Mar 9, 2024

fmease Mar 8, 2024

wutchzone Mar 9, 2024

fmease Mar 8, 2024 •

edited

Loading

wutchzone Mar 9, 2024 •

edited

Loading

wutchzone commented Mar 9, 2024

fmease Mar 9, 2024

fmease Mar 9, 2024

fmease commented Mar 9, 2024

This comment has been minimized.

bors commented Mar 9, 2024

bors commented Mar 9, 2024

This comment has been minimized.

rust-timer commented Mar 9, 2024

Kobzol commented Mar 10, 2024

fmease left a comment

fmease commented Mar 11, 2024

bors commented Mar 11, 2024

	PathStyle::Expr => "call the expresion",
	PathStyle::Expr => "call the expression",

Improve diagnostics for parenthesized type arguments #122152

Improve diagnostics for parenthesized type arguments #122152

Conversation

wutchzone commented Mar 7, 2024

rustbot commented Mar 7, 2024

This comment has been minimized.

fmease left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmease Mar 8, 2024 • edited Loading

Choose a reason for hiding this comment

wutchzone Mar 9, 2024 • edited Loading

Choose a reason for hiding this comment

wutchzone commented Mar 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmease commented Mar 9, 2024

This comment has been minimized.

bors commented Mar 9, 2024

bors commented Mar 9, 2024

This comment has been minimized.

rust-timer commented Mar 9, 2024

Overall result: ✅ improvements - no action needed

Kobzol commented Mar 10, 2024

fmease left a comment

Choose a reason for hiding this comment

fmease commented Mar 11, 2024

bors commented Mar 11, 2024

fmease left a comment •

edited

Loading

fmease Mar 8, 2024 •

edited

Loading

wutchzone Mar 9, 2024 •

edited

Loading